-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Line filters: Regex support #963
Line filters: Regex support #963
Conversation
…sitive-localstorage' into gtk-grafana/issues/952/line-filter-ui-updates__regex-option
…sitive-localstorage' into gtk-grafana/issues/952/line-filter-ui-updates__regex-option
…2/line-filter-ui-updates__regex-option
…2/line-filter-ui-updates__line-filter-var-v2
…2/line-filter-ui-updates__line-filter-var-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing feature. Left some minor comments.
src/Components/ServiceScene/LineFilter/LineFilterScene.test.tsx
Outdated
Show resolved
Hide resolved
…2/line-filter-ui-updates__line-filter-var-v2
…2/line-filter-ui-updates__line-filter-var-v2
Updated to no longer run queries on change, we can follow up with improvements to the debugging UX later. |
Met with @zizzpudding we agreed to kick the debug/editing mode to a follow-up PR, and we tweaked the style of the submit button(s) and removed the include/exclude dropdown from the line filter in the logs tab. Should be a bit more consistent with filters in other sections of the UI now. |
I would say this is ready for final review. Be nice to get this out in 1.0.5, but we can discuss tomorrow |
Hard agree with the include/exclude, specially after I saw it in patterns too. I'll take another look in the am. Great work! |
Stretch goal, but I would love if you can include a minimal validation to the input, so we can tell the user that something is going to produce errors beforehand: Same with the regex. If it would be possible to tell the user if the regex is valid or not before the query runs. Definitely as a follow up, but if we can release it along with this it would be perfect. |
Idea: could we use a query with line limit 1, not sure if instant would help, to let the user know that a line filter will produce no results before applying? (If the interaction gives time) |
The field tab seems to be acting up. Takes me a little bit but I manage to reproduce it to the end. Basically query with error > go to tabs > 0 fields > back to logs > remove error > fields stay in 0. Fields.mov |
Besides what I commented, this seems ready to be merged. |
Co-authored-by: Matias Chomicki <[email protected]>
Good catch, I forgot to subscribe to the line-filters and update the detected_fields on change. Will have a fix shortly. |
Modified the milestone to 1.0.6, because we're deploying today after the holiday code-freeze we'd like this to have some time in main before going out to users due to the size/complexity of this PR. Hopefully 1.0.6 can be released next week so it shouldn't be too long of a wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 👏
…s__line-filter-var-v2
Adds regex and exclusion support to the line filters.
Fixes: #742
Fixes: #421
Addresses: #758
Note
Breaking UX change:
Line filters must be submitted before they will run queries.
This PR:
Examples
Multiple line filters
Regex line filters
Exclusion filters from log line context menu
TODOs
Notes for your reviewers:
Part of #952
#958 wasn't going to work, see #958 (comment) for more detail.